[fix][broker] Ignore and remove the replicator cursor when the remote cluster is absent by BewareMyPower · Pull Request #19972 · apache/pulsar

BewareMyPower · 2023-03-30T17:12:17Z

Motivation

Sometimes when a remote cluster is deleted, the replication cursor might still exist for some topics. In this case, creating producers or consumers on these topics will fail.

Here is a log observed in a production environment:

WARN org.apache.pulsar.broker.service.BrokerService - Replication or
dedup check failed. Removing topic from topics list
persistent://public/__kafka/__consumer_offsets-partition-40,
java.util.concurrent.CompletionException: java.lang.RuntimeException:
org.apache.pulsar.metadata.api.MetadataStoreException$NotFoundException:
kop

If it happened, unloading the topic or restarting the broker could not help. We have to remove the cursor manually.

Modifications

In addReplicationCluster, before getting the replication client, check the namespace policy and topic policy first. If the remote cluster does not exist, skip adding the replication client and remove the cursor.

Verifications

PersistentTopicTest#testCreateTopicWithZombieReplicatorCursor is added to verify PersistentTopic#initialize will succeed and the zombie replicator cursor will be removed.

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: BewareMyPower#23

github-actions · 2023-03-30T17:12:53Z

@BewareMyPower Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

poorbarcode

ShadowReplicator hits this issue, too. Could you please also fix it?

BewareMyPower · 2023-03-31T04:40:34Z

ShadowReplicator hits this issue, too. Could you please also fix it?

I think it's another issue. You can open another PR for it.

… cluster is absent ### Motivation Sometimes when a remote cluster is deleted, the replication cursor might still exist for some topics. In this case, creating producers or consumers on these topics will fail. Here is a log observed in a production environment: > WARN org.apache.pulsar.broker.service.BrokerService - Replication or > dedup check failed. Removing topic from topics list > persistent://public/__kafka/__consumer_offsets-partition-40, > java.util.concurrent.CompletionException: java.lang.RuntimeException: > org.apache.pulsar.metadata.api.MetadataStoreException$NotFoundException: > kop If it happened, unloading the topic or restarting the broker could not help. We have to remove the cursor manually. ### Modificatons In `addReplicationCluster`, before getting the replication client, check the namespace policy and topic policy first. If the remote cluster does not exist, skip adding the replication client and remove the cursor. ### Verifications `PersistentTopicTest#testCreateTopicWithZombieReplicatorCursor` is added to verify `PersistentTopic#initialize` will succeed and the zombie replicator cursor will be removed.

BewareMyPower · 2023-03-31T09:33:29Z

Hi, @poorbarcode @315157973 Now I checked the namespace and topic policies to determine whether to delete the cursor.

codecov-commenter · 2023-04-03T10:46:15Z

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.85%. Comparing base (68c10ee) to head (335b4d6).
⚠️ Report is 2687 commits behind head on master.

Files with missing lines	Patch %	Lines
...sar/broker/service/persistent/PersistentTopic.java	94.44%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19972      +/-   ##
============================================
- Coverage     72.89%   72.85%   -0.05%     
+ Complexity    31619    31613       -6     
============================================
  Files          1861     1861              
  Lines        137356   137375      +19     
  Branches      15117    15120       +3     
============================================
- Hits         100131   100089      -42     
- Misses        29271    29329      +58     
- Partials       7954     7957       +3

Flag	Coverage Δ
inttests	`24.28% <50.00%> (-0.15%)`	⬇️
systests	`25.00% <0.00%> (-0.04%)`	⬇️
unittests	`72.15% <94.44%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...sar/broker/service/persistent/PersistentTopic.java	`80.15% <94.44%> (+0.24%)`	⬆️

... and 77 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

315157973 · 2023-04-03T11:20:08Z

+                .thenCompose(__ -> checkReplicationCluster(remoteCluster))
+                .thenCompose(clusterExists -> {
+                    if (!clusterExists) {
+                        log.warn("Remove the replicator because the cluster '{}' does not exist", remoteCluster);
+                        return removeReplicator(remoteCluster).thenApply(__ -> null);


Why don't we clean up when the Policy is removed?

Sorry I missed this comment when I merged. And yes, we should do that as well. WDYT? @poorbarcode

@315157973

Why don't we clean up when the Policy is removed?

When the policy is removed, the cursor will not be clean If this topic is not online. see

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

Line 1589 in d1fc732

futures.add(removeShadowReplicator(shadowTopic));

… cluster is absent (#19972) (cherry picked from commit d1fc732)

lhotari · 2023-04-04T20:49:40Z

This PR introduced a new flaky test, reported as #20010 . @BewareMyPower do you have a chance to take a look? thanks

BewareMyPower · 2023-04-05T14:19:53Z

Okay, I assigned that issue to me first.

Technoboy- · 2023-04-10T01:55:10Z

-        replicators.get(remoteCluster).disconnect().thenRun(() -> {
-
+        Optional.ofNullable(replicators.get(remoteCluster)).map(Replicator::disconnect)
+                .orElse(CompletableFuture.completedFuture(null)).thenRun(() -> {


orElseGet is better

I think not. See

public static <U> CompletableFuture<U> completedFuture(U value) { return new CompletableFuture<U>((value == null) ? NIL : value); }

so completedFuture(null) won't create a new instance.

… cluster is absent (#19972)

… cluster is absent (apache#19972) (cherry picked from commit d1fc732) (cherry picked from commit b93176c)

michaeljmarshall · 2023-06-27T21:56:02Z

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label

…ting logic for better handle the checkpoint. (apache#19972) * Change the initial start cursor and stop cursor to better handle the consuming behaviors. * Create the initial subscription instead seek every time. This should fix the wrong position setting. * Fix the wrong stop cursor, make sure it stops at the correct space * Drop Consumer.seek() for apache#16171

BewareMyPower added type/bug The PR fixed a bug or issue reported a bug area/broker area/geo-replication release/2.10.5 release/2.9.6 release/2.11.2 labels Mar 30, 2023

BewareMyPower added this to the 3.0.0 milestone Mar 30, 2023

BewareMyPower requested review from 315157973, Demogorgon314, codelipenghui, coderzc, eolivelli and poorbarcode March 30, 2023 17:12

BewareMyPower self-assigned this Mar 30, 2023

github-actions Bot added the doc-label-missing label Mar 30, 2023

github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 30, 2023

poorbarcode reviewed Mar 31, 2023

View reviewed changes

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java Outdated

315157973 reviewed Mar 31, 2023

View reviewed changes

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java Outdated

BewareMyPower marked this pull request as draft March 31, 2023 07:50

BewareMyPower force-pushed the bewaremypower/replicator-zombie-cursors branch from 1469b85 to 8aeb37d Compare March 31, 2023 09:30

BewareMyPower marked this pull request as ready for review March 31, 2023 09:31

poorbarcode approved these changes Apr 1, 2023

View reviewed changes

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

Add warn logs

335b4d6

315157973 approved these changes Apr 3, 2023

View reviewed changes

315157973 reviewed Apr 3, 2023

View reviewed changes

BewareMyPower merged commit d1fc732 into apache:master Apr 4, 2023

BewareMyPower added a commit that referenced this pull request Apr 4, 2023

[fix][broker] Ignore and remove the replicator cursor when the remote…

b93176c

… cluster is absent (#19972) (cherry picked from commit d1fc732)

BewareMyPower added the cherry-picked/branch-2.10 label Apr 4, 2023

lhotari mentioned this pull request Apr 5, 2023

Flaky-test: PersistentTopicTest.testCreateTopicWithZombieReplicatorCursor #20010

Closed

2 tasks

liangyepianzhou added release/2.10.4 and removed release/2.10.5 labels Apr 6, 2023

poorbarcode mentioned this pull request Apr 6, 2023

[fix] [broker] Remove the repl cursor if the creation fails and the replication is turned off #20025

Closed

4 tasks

Technoboy- reviewed Apr 10, 2023

View reviewed changes

Technoboy- pushed a commit that referenced this pull request May 9, 2023

[fix][broker] Ignore and remove the replicator cursor when the remote…

9948f89

… cluster is absent (#19972)

Technoboy- added the cherry-picked/branch-2.11 label May 9, 2023

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 11, 2023

[fix][broker] Ignore and remove the replicator cursor when the remote…

77190ce

… cluster is absent (apache#19972) (cherry picked from commit d1fc732) (cherry picked from commit b93176c)

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

poorbarcode mentioned this pull request Jun 11, 2024

[fix] [broker] Messages lost on the remote cluster when using topic level replication #22890

Merged

4 tasks

Conversation

BewareMyPower commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifications

Documentation

Matching PR in forked repository

Uh oh!

github-actions Bot commented Mar 30, 2023

Uh oh!

poorbarcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BewareMyPower commented Mar 31, 2023

Uh oh!

Uh oh!

BewareMyPower commented Mar 31, 2023

Uh oh!

Uh oh!

codecov-commenter commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

315157973 Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

BewareMyPower Apr 4, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Apr 4, 2023

Choose a reason for hiding this comment

Uh oh!

lhotari commented Apr 4, 2023

Uh oh!

BewareMyPower commented Apr 5, 2023

Uh oh!

Technoboy- Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

BewareMyPower Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall commented Jun 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

BewareMyPower commented Mar 30, 2023 •

edited

Loading

codecov-commenter commented Apr 3, 2023 •

edited

Loading